Skip to content

Validate aggregation callables with single-argument bind check#290

Merged
CodeReclaimers merged 3 commits intoCodeReclaimers:masterfrom
shuofengzhang:fix/aggregation-single-arg-validation-20260315
Mar 15, 2026
Merged

Validate aggregation callables with single-argument bind check#290
CodeReclaimers merged 3 commits intoCodeReclaimers:masterfrom
shuofengzhang:fix/aggregation-single-arg-validation-20260315

Conversation

@shuofengzhang
Copy link
Contributor

What changed

  • tightened validate_aggregation so custom aggregation callables must be invokable with a single positional argument (the iterable of node inputs)
  • replaced the previous positional-parameter presence check with inspect.Signature.bind validation using one positional argument
  • added a regression test (test_bad_add4) to reject two-required-argument aggregation functions at registration time

Why

  • aggregation functions are always called with one argument in runtime paths (aggregation(node_inputs))
  • before this change, functions that required two positional arguments could be registered and only fail later during network execution
  • failing fast at config/registration time provides clearer feedback and prevents hard-to-debug runtime errors

Testing

  • source .venv/bin/activate && pytest -q tests/test_aggregation.py
  • source .venv/bin/activate && pytest -q

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

- Error message now says "one required positional argument" (functions
  with extra optional args are still valid)
- Added comment explaining why builtins skip signature validation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@CodeReclaimers CodeReclaimers merged commit 04a7371 into CodeReclaimers:master Mar 15, 2026
@sonarqubecloud
Copy link

@CodeReclaimers
Copy link
Owner

Thanks for the pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants